Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove BaseHTTPMiddlewares , Ensure origin host is used in STAC links #190

Merged
merged 15 commits into from
Apr 12, 2024

Conversation

joshimai
Copy link
Contributor

@joshimai joshimai commented Mar 27, 2024

Description

Removing BaseHTTPMiddleware to improve performance. Re-implementation of timeout middleware and error handling middleware replaced by registering exception handlers with the app. Reimplementation of trace middleware by calling the tracing function directly instead of using a BaseHTTPMiddleware .

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
I ran ./scripts/test

Checklist:

Please delete options that are not relevant.

  • Unit tests pass locally (./scripts/test)
  • Code is linted and styled (./scripts/format)

@joshimai
Copy link
Contributor Author

performance with mainimage
performance with improvements
image

Copy link
Contributor

@TomAugspurger TomAugspurger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked through the timeout and exception handling changes, and I think we're good. Just one quick question.

And I'm less familiar with the Brotli middleware, but I think that looks OK too.

return await handle_exceptions(request, call_next)


app.add_exception_handler(Exception, http_exception_handler)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that if you make an invalid POST request (like bad data) we still get the nice error message? I couldn't quickly confirm what the behavior of app.add_exception_handler is with Exception subclasses / order. I see that we reraise HTTPException in our handler, and just want to make sure FastAPI's still handles it.

Copy link
Contributor Author

@joshimai joshimai Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
okie so this is an instance of internal server error

Copy link
Contributor Author

@joshimai joshimai Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

imageimage
this is 404 collection not found

@mmcfarland
Copy link
Member

Also taking a look at this.

pcstac/pcstac/main.py Outdated Show resolved Hide resolved
@ghidalgo3 ghidalgo3 mentioned this pull request Mar 28, 2024
5 tasks
@ghidalgo3
Copy link
Contributor

You will need #195 to fix the CI build

pccommon/pccommon/middleware.py Show resolved Hide resolved
pcstac/pcstac/main.py Outdated Show resolved Hide resolved
pcstac/pcstac/main.py Outdated Show resolved Hide resolved
pccommon/pccommon/tracing.py Outdated Show resolved Hide resolved
pccommon/pccommon/tracing.py Outdated Show resolved Hide resolved
pccommon/pccommon/tracing.py Show resolved Hide resolved
pccommon/pccommon/tracing.py Outdated Show resolved Hide resolved
pccommon/pccommon/tracing.py Show resolved Hide resolved
pccommon/pccommon/middleware.py Outdated Show resolved Hide resolved
pccommon/pccommon/middleware.py Show resolved Hide resolved
pctiler/pctiler/main.py Outdated Show resolved Hide resolved
@joshimai joshimai merged commit f983bea into main Apr 12, 2024
3 checks passed
@joshimai joshimai deleted the joshi/performance-improvements branch April 12, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants